Table features interfaces/utilities for Kernel #4

vkorukanti · 2025-02-11T17:36:52Z

Which Delta project/connector is this regarding?

Description

How was this patch tested?

Does this PR introduce any user-facing changes?

scottsand-db

Looks great! Super detailed and thorough. Thanks for this! Left some comments.

scottsand-db · 2025-02-11T17:41:04Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/TableConfig.java

+          true);
+
+  /** Whether commands modifying this Delta table are allowed to create new deletion vectors. */
+  public static final TableConfig<Boolean> ENABLE_DELETION_VECTORS_CREATION =


nit: would it be nice if all of our table-feature-enablement-related TableConfigs ended with _ENABLED?

e.g. APPEND_ONLY_ENABLED, CHANGE_DATA_FEED_ENABLED, DELETION_VECTORS_CREATION_ENBABLED

scottsand-db · 2025-02-11T18:25:09Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/tablefeatures/TableFeature.java

+ *
+ * <p>Separately, a feature can be automatically supported by a table's metadata when certain
+ * feature-specific table properties are set. For example, `changeDataFeed` is automatically
+ * supported when there's a table property `delta.enableChangeDataFeed=true`. See {@link


does this mean that in the metadata / protocol that is written to the lob, delta.enableChangeDataFeed=true is in the metadata (table property) but changeDataFeed is not in the protocol's reader/writer features?

so do we just, implicitly, when we create the Protocol action, we add that changeDataFeed reader/write feature?

and then do we write out the new-and-improved-and-corrected Protocol next time we write?

does this mean that in the metadata / protocol that is written to the lob, delta.enableChangeDataFeed=true is in the metadata (table property) but changeDataFeed is not in the protocol's reader/writer features?

Thats is correct.

so do we just, implicitly, when we create the Protocol action, we add that changeDataFeed reader/write feature?

Yep, if all the metadata requirements are satisfied.

and then do we write out the new-and-improved-and-corrected Protocol next time we write?

Yes

scottsand-db · 2025-02-11T19:14:32Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/tablefeatures/TableFeature.java

+   * @param featureName a globally-unique string indicator to represent the feature. All characters
+   *     must be letters (a-z, A-Z), digits (0-9), '-', or '_'. Words must be in camelCase.
+   * @param minReaderVersion the minimum reader version this feature requires. For a feature that
+   *     can only be explicitly supported, this is either `0` or `3` (the reader protocol version


also remind us what 0 means -->

0 (this is a writer feature, not a reader feature) or 3 (the reader protocol ....)

scottsand-db · 2025-02-11T19:15:27Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/tablefeatures/TableFeature.java

+    checkArgument(
+        featureName.chars().allMatch(c -> Character.isLetterOrDigit(c) || c == '-' || c == '_'),
+        "name contains invalid characters: " + featureName);
+    this.minReaderVersion = minReaderVersion;


checkArgument >= 0?

scottsand-db · 2025-02-11T19:15:48Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/tablefeatures/TableFeature.java

+        featureName.chars().allMatch(c -> Character.isLetterOrDigit(c) || c == '-' || c == '_'),
+        "name contains invalid characters: " + featureName);
+    this.minReaderVersion = minReaderVersion;
+    this.minWriterVersion = minWriterVersion;


whats the min? checkArgument >= 2? or whaterver it is supposed to be

It is possible to have writer version 1.

scottsand-db · 2025-02-11T19:38:45Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/tablefeatures/TableFeatures.java

+    }
+  }
+
+  public static final TableFeature VARIANT_FEATURE = new VariantTypeTableFeature("variantType");


what are your thoughts on VARIANT_RW_FEATURE?

and then DOMAIN_METADATA_W_FEATURE ?

i.e. include the readerWriter or writer-ness in the variable name itself? this could help throughout the code to re-enforce understanding of the features?

no prefernece. Added the prefixes.

scottsand-db · 2025-02-11T19:40:40Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/actions/Protocol.java

+            // can be explicitly added to the protocol's `readerFeatures`
+            supportsReaderFeatures();
+
+    boolean shouldAddToWriterFeatures = supportsWriterFeatures();


don't we need to check if the feature is a writer feature?

scottsand-db · 2025-02-11T19:42:06Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/actions/Protocol.java

+   * Determine whether this protocol can be safely upgraded to a new protocol `to`. This means: -
+   * all features supported by this protocol are supported by `to`.
+   *
+   * <p>Examples regarding feature status: - from `[appendOnly]` to `[appendOnly]` => allowed. -


nit: the - bullet points should be on newlines

scottsand-db · 2025-02-11T19:43:50Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/actions/Protocol.java

+   */
+  public Protocol normalized() {
+    // Normalization can only be applied to table feature protocols.
+    if (!supportsWriterFeatures()) {


what about readerFeatures? is there a relationship between them? i.e. does supportsReaderFeatures imply supportsWriterFeatures?

scottsand-db · 2025-02-11T19:45:46Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/actions/Protocol.java

+   * <p>For example: (1, 7, AppendOnly, Invariants, CheckConstraints) -> (1, 3) (3, 7, RowTracking)
+   * -> (1, 7, RowTracking)
+   */
+  public Protocol normalized() {


is normalize the right/best word here? wasn't obvious to me. what about minimized?

and then maximized for denormalized

allisonport-db · 2025-02-11T20:11:06Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/tablefeatures/TableFeatures.java

+  }
+
+  /** A base class for all table legacy reader-writer features. */
+  public abstract static class LegacyReaderWriterFeature extends TableFeature


should this implement ReaderWriterFeatureType?

I think it must? Otherwise isReaderWriterFeature is wrong?

yep, added it.

allisonport-db · 2025-02-11T20:12:02Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/tablefeatures/TableFeature.java

+  public boolean isReaderWriterFeature() {
+    return this instanceof TableFeatures.ReaderWriterFeatureType;
+  }


Maybe a silly question, but do we need this interface at all? Can we just infer this from minReaderVersion?

I think in your doc you mentioned this might do a check and require that minReaderVersion is overriden to be non-zero?

Hmm I guess I see it is used below in validate. Still wondering what value the interface uses though and if we would just always use this method rather than checking instanceof

It is a descriptive way. When you see a table feature and what is inheirting, we know it is a readerwriter or writer only feature.

we could depend on minReaderVersion = 0, but that's just hacky.

vkorukanti added 3 commits February 6, 2025 00:31

Table feature interfaces

6aae232

interface updates

a3e08c2

utility methods

f00cd11

scottsand-db suggested changes Feb 11, 2025

View reviewed changes

allisonport-db reviewed Feb 11, 2025

View reviewed changes

vkorukanti added 2 commits February 11, 2025 14:24

fix

6a816fa

updates

c905fa0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Table features interfaces/utilities for Kernel #4

Table features interfaces/utilities for Kernel #4

vkorukanti commented Feb 11, 2025

scottsand-db left a comment

scottsand-db Feb 11, 2025

vkorukanti Feb 14, 2025

scottsand-db Feb 11, 2025

vkorukanti Feb 14, 2025

scottsand-db Feb 11, 2025

vkorukanti Feb 14, 2025

scottsand-db Feb 11, 2025

vkorukanti Feb 14, 2025

scottsand-db Feb 11, 2025

vkorukanti Feb 14, 2025

scottsand-db Feb 11, 2025

vkorukanti Feb 14, 2025

scottsand-db Feb 11, 2025

scottsand-db Feb 11, 2025

scottsand-db Feb 11, 2025

scottsand-db Feb 11, 2025

scottsand-db Feb 11, 2025

allisonport-db Feb 11, 2025

allisonport-db Feb 11, 2025

vkorukanti Feb 14, 2025

allisonport-db Feb 11, 2025

allisonport-db Feb 11, 2025

allisonport-db Feb 11, 2025

vkorukanti Feb 14, 2025

Table features interfaces/utilities for Kernel #4

Are you sure you want to change the base?

Table features interfaces/utilities for Kernel #4

Conversation

vkorukanti commented Feb 11, 2025

Which Delta project/connector is this regarding?

Description

How was this patch tested?

Does this PR introduce any user-facing changes?

scottsand-db left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment